Skip to content

Fix TypeBase::hasOpaqueArchetypePropertiesOrCases for recursive types #34077

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

aschwaighofer
Copy link
Contributor

Also fix the code handling enums to use getArgumentInterfaceType instead
of getInterfaceType and handle tuples.

rdar://68798822

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - c8a3aeeda7950481bf35f033cd043857c7b9e536

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - c8a3aeeda7950481bf35f033cd043857c7b9e536

Copy link
Contributor

@slavapestov slavapestov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be computed as a SIL type lowering property instead?

lib/AST/Type.cpp Outdated
if (eltType &&
(eltType->hasOpaqueArchetype() ||
eltType->getCanonicalType()->hasOpaqueArchetypePropertiesOrCasesImpl(
visited)))
return true;
}
}
Copy link
Contributor

@rjmccall rjmccall Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also need a case for tuples here? When the opaque archetype is directly within the tuple, it'll be found by hasOpaqueArchetype(), but if you have a tuple containing a struct that has an opaque archetype in a property, you won't find that.

hasOpaqueArchetype() will also return true for things that don't store opaque archetypes, like if you have a function that returns an opaque archetype. I don't know if that's a problem, because I don't really understand the purpose of this.

To know whether something stores an opaque archetype, you really need to do substitution to get the type of the property/case. You can just use getTypeOfMember() for that. That opens you up to a problem where you can have an infinite tower of types, e.g. with:

enum OptionalTower<T> {
  case base
  indirect case next(Tower<T?>)
}

In valid code, you may be able to handle that by ignoring indirect cases, since the values of indirect cases are not directly stored in an enum. Again, I really don't know what the purpose of this function is. Maybe we just need to talk about what this is used for, and we can find a better alternative?

Copy link
Contributor Author

@aschwaighofer aschwaighofer Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also need a case for tuples here?

Yes it is needed. I had added tuple types based on your feedback in the radar. It is already there.

This code is used in front of the type lowering cache. If a type can lower to different type lowerings (e.g loadable vs non-loadable) depending on the resilience expansion (or type context for opaque result types) we look for the right type lowering by type expansion context, if a type can't lower to different type lowerings (either because it is not resilient (recursively) or because it does not 'contain' opaque result types) we map to one type lowering.

  auto *candidateLowering = find(key.getKeyForMinimalExpansion());              
  auto *lowering = getTypeLoweringForExpansion(                                 
      key, forExpansion, candidateLowering, origHadOpaqueTypeArchetype); 
const TypeLowering *TypeConverter::                                             
getTypeLoweringForExpansion(TypeKey key,                                        
                            TypeExpansionContext forExpansion,                  
                            const TypeLowering *lowering,                       
                            bool origHadOpaqueTypeArchetype) {                  
  if (lowering == nullptr)                                                      
    return nullptr;                                                             
                                                                                
  if (!lowering->isResilient() && !origHadOpaqueTypeArchetype) {                
    // Don't try to refine the lowering for other resilience expansions if      
    // we don't expect to get a different lowering anyway. Similar if the       
    // original type did not have opaque type archetypes.                       
    //                                                                          
    // See LowerType::handleResilience() for the gory details; we only          
    // set this flag if the type is resilient *and* inside our module.          
    return lowering;                                                            
  }                                                                             
                                                                                
  auto *exactLowering = find(key);                                              
  if (exactLowering)                                                            
    return exactLowering;   

For this purpose, it is okay for hasOpaqueArchetypePropertiesOrCases to eagerly answer yes. But it is not okay to answer no when there is an opaque result type that influence type lowering depending on whether we look through to the underlying type or not.

Yes, as you say for this purpose we can ignore indirect enum cases because the argument type of indirect cases will not influence the lowering since it is a reference.

@slavapestov's point is good. Just like resilience this can also be compute as a recursive type lowering property. (This way it will be more precise then my existing code, because we will handle the cases where the opaque result type is not 'stored').
I will do that instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the way type lowering works with resilience is that after we lower the type (which recursively walks stored properties and performs substitutions, as @rjmccall was asking about), then we check if the lowering we produced depends on resilience. If that is the case, we can lower it again with a different expansion. The same idea should work with opaque types.

I'm hesitant to add new walks over stored properties and enum cases because it's always tricky to handle recursion and substitutions. Ideally we should centralize such walks as much as possible.

…d of TypeBase's hasOpaqueArchetypePropertiesOrCases

A type is IsTypeExpansionSensitive if it contains an opaque result type that influences how the type is lowered.

This could be because the type mentions an opaque archetype and
therefore we would look through to the underlying type depending on the
type expansion and could get a different SIL type. For example
'() -> out some P' could lower to '() -> @out Int'.

Or this could be because when we lower an aggregate type some of its
fields are opaque types that could be looked through and therefore the
aggregate has different lowering (e.g address vs. loadable) in different
type expansion contexts.

By replacing it this change also fixes an infinite recursion in
hasOpaqueArchetypePropertiesOrCases.

rdar://68798822
@aschwaighofer aschwaighofer force-pushed the fix_hasOpaqueArchetypePropertiesOrCases branch from c8a3aee to 8e8c57c Compare September 29, 2020 17:41
@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8e8c57c

@aschwaighofer
Copy link
Contributor Author

@swift-ci please test os x

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test source compatibility

1 similar comment
@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test source compatibility

@aschwaighofer
Copy link
Contributor Author

@swift-ci please test source compatibility

@aschwaighofer
Copy link
Contributor Author

@swift-ci Please test source compatibility

@aschwaighofer
Copy link
Contributor Author

The only failures in the source compatibility debug run are unexpected passes:

UPASS_BlueSocket_4.2_BuildXcodeWorkspaceScheme_Socket-iOS_generic-platform-iOS.log
UPASS_BlueSocket_5.0_BuildXcodeWorkspaceScheme_Socket-iOS_generic-platform-iOS.log
UPASS_BlueSocket_5.1_BuildXcodeWorkspaceScheme_Socket-iOS_generic-platform-iOS.log

These are all marked as passing on the latest debug run: https://ci.swift.org/job/swift-main-source-compat-suite-debug/3549/artifact/swift-source-compat-suite/.

@aschwaighofer aschwaighofer merged commit 913543e into swiftlang:main Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants